-
Notifications
You must be signed in to change notification settings - Fork 589
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PrintBGZFBlockInformation: remove file extension check so that we can accept, eg., bams #5801
Conversation
… accept, eg., bams The file extension check in this tool was redundant, since there is a subsequent check that the input file is in BGZF format. Resolves #5800
@jamesemery please review (@kvg ran into this issue yesterday) |
Codecov Report
@@ Coverage Diff @@
## master #5801 +/- ##
===============================================
- Coverage 87.002% 85.583% -1.419%
+ Complexity 32111 31394 -717
===============================================
Files 1974 1977 +3
Lines 147246 147095 -151
Branches 16217 16170 -47
===============================================
- Hits 128107 125888 -2219
- Misses 13233 15211 +1978
- Partials 5906 5996 +90
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly some pedantic documentation requests.
@@ -71,11 +71,6 @@ protected void onStartup() { | |||
throw new UserException.CouldNotReadInputFile("File " + bgzfPathString + " does not exist"); | |||
} | |||
|
|||
if ( ! IOUtil.hasBlockCompressedExtension(bgzfPathString) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you should amend the below statement "File is not a valid BGZF file. Could possibly be a regular GZIP file?" to better reflect that the user may have provided any file under the sun.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also explicitly add a reference to the javadocs for this tool that mentions the bam use case as it sounds incredibly useful to people.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done and done
@@ -100,4 +100,20 @@ public void testRegularGzipFile() throws IOException { | |||
}; | |||
runCommandLine(args); | |||
} | |||
|
|||
/* Make sure that we can handle a standard BAM file */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a test to ensure there is a nice exception when the user provides a non-BGZF file (maybe a VCF).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@jamesemery Comments addressed, back to you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good 👍
The file extension check in this tool was redundant, since there is a subsequent check
that the input file is in BGZF format.
Resolves #5800